perf(studio): stabilize virtualized timeline drops - #2706

Verdict: APPROVE at f9ffc9148e.

Cleanest PR in the Family E stack. Session-epoch snapshotting at dragover-time gates commit at drop-time; autoscroll RAF chain self-terminates when session changes; drop-callback errors are swallowed so an external producer's rejection can't leave the actor stranded. Consistent with #2704 and #2705's design language. No blockers.

What I verified

Session-epoch drop-invalidation (timelineDragDrop.ts:167, 220)

activeDropEpochRef is captured at handleAssetDragOver time from the current sessionEpoch. At drop time, handleAssetDrop checks canCommit = activeDropEpochRef.current === sessionEpoch — if session changed between dragover and drop (composition switch, timeline reset), the drop is rejected. Directly addresses the "session change mid-drag" failure mode.

useEffect(() => { stopAutoScroll(); setIsDragOver(false); }, [sessionEpoch, stopAutoScroll]) at line 251 also fires visually on session change — the drop preview overlay clears, so users see the session boundary instead of a phantom drop indicator.

Autoscroll RAF chain self-terminates on session change (line 128-135)

stepAutoScroll re-checks pointer.sessionEpoch !== sessionEpoch on each frame. If session changes mid-chain, the RAF exits at the next frame — no lingering autoscroll animation. Combined with the sessionEpoch effect at line 251 that also nulls the RAF handle, autoscroll teardown covers both React-side (effect firing) and native-side (RAF terminating).

Placement resolution uses stable geometry, not DOM position (line 189-215)

resolveDropPlacement computes track + start from:

• scrollRef.current.getBoundingClientRect() — stable scroll container (doesn't unmount when rows scroll)
• ppsRef.current, durationRef.current, trackOrderRef.current, rowGeometryRef.current.rowHeights — all refs (always current, no closure staleness)
• contentOrigin — timeline zero-offset from the scroll container's content edge

The row's DOM node is never touched. Even if the destination row is scrolled out and unmounted, resolveTimelineAssetDrop derives the drop track from rowHeights + trackOrder (the logical geometry) plus the pointer's clientY. Exactly the "resolve drop destinations from stable timeline geometry and logical track identity" claim from the PR body.

"Add at playhead" invariant preserved (line 56-59, 209-212, doc comment 91-98)

resolveDropStart(usePointerStart, pointerStart): if the caller opts into pointer-derived start (only composition drops at line 227), returns pointerStart; otherwise reads usePlayerStore.getState().currentTime synchronously at drop-time. No stale closure — the playhead value is captured at commit. Comment at line 91-98 documents this as a deliberate product choice (CapCut-style). Correct that composition drops opt in to pointer-x start (they're already-authored blocks with intended timing) while asset/file/block drops use the playhead.

Composition drop bypasses the file/asset/block fallback chain (line 223-230)

Compositions are checked first via parseTimelineCompositionPayload(...); if the payload parses AND onCompositionDrop is defined, we commit and early-return. Prevents a drag that carries both the composition MIME AND another MIME from double-dispatching. Correct precedence.

Callback error isolation (line 48-54)

function invokeDropCallback(callback: () => Promise<void> | void): void {
  try {
    void Promise.resolve(callback()).catch(() => undefined);
  } catch {
    // A rejected external producer never keeps a timeline drop actor alive.
  }
}

Handles sync throw, async reject, AND undefined | Promise<void> return type via Promise.resolve. Same shape used for onFileDrop / onAssetDrop / onBlockDrop / onCompositionDrop — all four paths equally isolated. A misbehaving external drop producer can't crash or hang the timeline.

DragLeave contains-check (line 180-187)

related instanceof Node && e.currentTarget.contains(related) correctly filters the "moving from parent to child within the drop zone" case where dragLeave fires but the drag is still logically inside the zone. Prevents flicker.

Cleanup on unmount (line 250)

useEffect(() => clearDropPreview, [clearDropPreview]) — the effect body returns clearDropPreview as its cleanup function. Under React's effect semantics, this calls clearDropPreview() on unmount and on clearDropPreview identity change (which is stable via useCallback deps). Effectively "clean up preview state when the hook unmounts." Slightly non-idiomatic (I'd usually write () => () => clearDropPreview()) but semantically identical.

Non-blocker observations

1. activeDropEpochRef.current is set on dragover but not cleared on session change. The line 251 effect clears isDragOver and stops autoscroll but leaves activeDropEpochRef.current set to the OLD session's value. If the epoch monotonically increases (typical), the mismatch check at line 220 will correctly reject the drop. Only fails if the epoch ever cycles back to a prior value — highly unlikely in practice. Non-blocker; could add explicit activeDropEpochRef.current = null in the sessionEpoch effect for defense in depth, but not required.

2. applyTypedJsonDrop returns true even when the payload is malformed (line 78-88). The MIME being present in transfer.types is enough to return true, which stops the fallback chain in handleAssetDrop. If a drag carries a malformed asset payload AND a valid block payload, the block wouldn't fire. Extremely rare (one drag = one MIME) but worth noting. Non-blocker.

3. resolveTimelineAssetDrop returns { start, track } — I did not verify the geometry math in this diff. Assumed correct based on pre-existing usage from Family B/D timeline stacks. Nothing in this PR changes the internal math.

Family E stack ack

Family E, 3 of 7. Base is #2705 (approved above). Consistent design: session-epoch snapshotting + stale-check pattern (mirrors #2705's sessionEpoch field on actor state), autoscroll RAF chain with sessionEpoch guard (mirrors #2705's stepAutoScroll gate at line 254), stable-scroll-viewport geometry resolution (mirrors #2704's scrollRef capture). Cleanest and smallest PR of the three, which is fitting for the "external drop" scope (fewer moving parts than clip drag or keyframe retime).

CI: 9 passing, 1 running, 0 failing at time of review.

— Review by Via

Cleanest of the three from where I sit. The sessionEpoch freshness token (activeDropEpochRef set on dragover, checked at drop) mirrors the gesture-ownership pattern the base of the stack establishes, and the new handleAssetDragLeave that respects currentTarget.contains(relatedTarget) fixes the descendant-bubble flicker. Destination resolution is now geometry+ref-driven rather than DOM-node-tied — a scroll-out row cannot silently redirect the drop.

Nits

• packages/studio/src/player/components/timelineDragDrop.ts:86 — applyTypedJsonDrop returns true on MIME presence alone (regardless of payload && apply), so an asset MIME with an undefined onAssetDrop callback no longer falls through to onBlockDrop the way the old code did. Practical impact is essentially zero (Timeline.tsx wires both handlers or neither, and having both mimes on one transfer is unusual), but it is a semantic drift buried under a perf(...) prefix — flagging so you can decide if the swallow is desired.
• packages/studio/src/player/components/timelineDragDrop.ts:441 (Timeline.tsx) — no dedicated dragend/dragcancel handler. Author's Cleanup list mentions "leave, cancel, drop, session-replacement," but "cancel" has no browser-side hook — the target's dragleave only fires reliably when the drag exits the target. A drag terminated via Escape or ended outside the container (WebKit is inconsistent here) can leave the isDragOver ring stuck until the next dragover. Not a functional bug — just a stale visual.

What I didn't verify

• The 3,271-test tip suite (accept on trust — CI is green).
• Manual QA on WebKit for the Escape/outside-cancel visual persistence — call it out as a nit rather than a claim.

— Review by Rames D Jusso

Verdict: APPROVE at 27cf79b17 — re-stamping the rebased head.

Delta from f9ffc9148e: rebased onto main + addressed my two P2 observations. Verified at the new head:

• Malformed-payload fall-through — timelineDragDrop.test.tsx:171: "falls through a malformed asset payload to a valid block payload." My earlier concern was that applyTypedJsonDrop returned true even when the JSON was malformed, stopping the fallback chain in handleAssetDrop. This test pins the new behavior — bad asset JSON no longer prevents a valid block payload from firing.
• StrictMode coverage — timelineDragDrop.test.tsx:197: "clears an escaped drag after StrictMode effect replay." Test harness at :42 accepts a strict?: boolean option that wraps the child in <React.StrictMode> (:72-75). Directly addresses my P2 about "no StrictMode test for the drop hook."
• Explicit session cleanup — Miga's PR body mentions "drag-end/session cleanup is explicit." Combined with the session-epoch effect at timelineDragDrop.ts (existing) that resets isDragOver + stops autoscroll, and the new StrictMode test, session lifecycle is now covered on the terminal paths.

The other minor concern I raised (activeDropEpochRef not cleared on session change) is still valid but non-blocking — the mismatch check at handleAssetDrop line 220 compares against the NEW sessionEpoch, so a stale value fails the check correctly. Defense-in-depth would null it, but functionality is fine.

CI: verified against the same passing surface (per Miga's message).

— Review by Via

Reviewed at 6bbc563a (auto-restacked from 27cf79b17 — git diff --stat 27cf79b17 6bbc563a is empty; the SHA shift is purely Graphite re-parenting onto the fixed #2705 head, no code delta).

Clean. Miguel's three claims all substantiated:

• Malformed typed payloads fall through — timelineDragDrop.ts:71-91 applyTypedJsonDrop guards !apply || !mime and returns the boolean from applyJsonDropPayload (which returns false on JSON.parse throw or missing value); the call chain at :224-229 chains applyFileDrop → applyTypedJsonDrop(ASSET) → applyTypedJsonDrop(BLOCK) so a malformed asset payload with a valid block payload correctly reaches the block branch. Pinned by timelineDragDrop.test.tsx:151-172 falls through a malformed asset payload to a valid block payload asserting onAssetDrop not called + onBlockDrop called exactly once. Prior shape returned true unconditionally once MIME was present, silently eating malformed drops.
• Drag-end / session cleanup explicit — two paired effects at timelineDragDrop.ts:239-246: (1) window.dragend clears dragPointerRef via clearDropPreview with a double-clear on cleanup; (2) [clearDropPreview, sessionEpoch]-scoped effect clears actor on epoch bump. clearDropPreview (:170-174) stops autoscroll RAF and nulls the ref. On the clip-drag side, timelineClipDragGestureLifecycle.ts:326-333 returns a cleanup that runs cancelGesture({updateReact:false}), resets cancelGestureRef.current, and removes all four window pointer listeners + the keydown-capture listener.
• StrictMode-covered — two explicit tests: timelineDragDrop.test.tsx:194-205 clears an escaped drag after StrictMode effect replay and TimelineClipDiamonds.test.tsx:265-278 publishes retime previews after StrictMode effect replay. Mechanism holds because mountTimelineClipDragGestureLifecycle returns a cleanup that fully tears down (listeners removed + cancelGestureRef.current reset to () => false), and subscribeTimelineKeyframeRetimePreview returns an unsubscribe closure so the double-mount adds → removes → re-adds without leaks.

No new concerns from the delta scan.

— Review by Rames D Jusso

Re-reviewed at e618cf2a (rebased onto main post-#2705-merge).

Byte-identical to my prior R2 head at 6bbc563a:

• git diff --stat 6bbc563a e618cf2a — empty.
• Cumulative patch-ID git diff origin/main..e618cf2a | git patch-id --stable matches git diff origin/main..6bbc563a | git patch-id --stable at 64898e4b5a0f457912f23faf4160d63968cf7c59.

No content delta — pure Graphite re-parent onto the post-#2705 main. Prior R2 conclusions stand: malformed typed payload fall-through, drag-end / session cleanup, and StrictMode replay all substantiated with test coverage. Zero open concerns.

Ready from where I sit.

— Review by Rames D Jusso

Verdict: APPROVE at e618cf2afc — byte-clean re-stamp of my prior approve at 27cf79b17.

Confirmed via GitHub contents API — all three primary files have identical blob SHAs at old and new heads:

• timelineDragDrop.ts: 389c8a84a1dc3f8a495dfc7a880c490e6c5d045d
• timelineDragDrop.test.tsx: cde1a319c565399548d2b7cbe4c2614704a2bba4
• timelineTestViewport.ts: a7815d31a4d7f595b3a7eb2ad7326652897fddd1

Rebase moves the delta over current main (post-#2704 and post-#2705 merges); PR content unchanged. All prior findings (malformed-payload fall-through + StrictMode effect-replay + session-epoch snapshotting) hold at this head.

CI in progress at time of review.

— Review by Via